fix: Use a consistent evaluation context for hook invocations and prerequisite evaluations. - #381
fix: Use a consistent evaluation context for hook invocations and prerequisite evaluations.#381abelonogov-ld wants to merge 2 commits into
Conversation
The ten variation methods each spelled out the same call to the hook runner around the same evaluation, and left the flag read to the evaluation itself. They now collapse onto a helper that does the read, so one place expresses the order of the read, the hooks and the evaluation, and what an evaluation is about to return can be described without reading the store a second time. Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 7efd98c. Configure here.
evaluateWithHooks snapshotted the flag before hooks ran, but variationDetailInternal still re-read the evaluation context when recording events. An identify landing in between left the returned value from the prior context's flag attributed to the new context. Both are now read together and threaded through the evaluation so the series, the result and the events all describe one pair. Co-authored-by: Cursor <cursoragent@cursor.com>
|
This seems like an improvement, but there are related cases that are not fixed (nor are they trivial to fix). The related cases are a prerequisite graph where a diamond exists. A -> B, A -> C, B -> D, C -> D. I think even with this change, D will be read twice from the store. A proper fix would be some sort of snapshot of storage, but that is much more involved. |
| private EvaluationDetail<LDValue> variationDetailInternal(@NonNull String key, @NonNull LDValue defaultValue, boolean checkType, boolean needsReason, Set<String> visited) { | ||
| LDContext context = clientContextImpl.getEvaluationContext(); | ||
| Flag flag = contextDataManager.getNonDeletedFlag(key); // returns null for nonexistent *or* deleted flag | ||
| private EvaluationDetail<LDValue> variationDetailInternal(@NonNull String key, @NonNull LDValue defaultValue, boolean checkType, boolean needsReason, Set<String> visited, @Nullable Flag flag, @NonNull LDContext context) { |
There was a problem hiding this comment.
Do you even need the flag key param now (the first param)? It seems like this function just operates on the domain object Flag and not on the key since the lookup was done beforehand.
kinyoklion
changed the title
Updated the PR description to note the limitations, but still highlight the value of the change. |
|
This was superseded by PR 380 that contained all the same changes. So I am closing this PR. |
3 participants
There are two primary purposes of this PR.
Currently the context and the store data are not transactional with each other. So there are limits to the approach, but it is generally an improvement regardless.